-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Bluetooth: Controller: llcp: CTE request: Add handling and tests for LL_UNKNOWN_RSP #45244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bluetooth: Controller: llcp: CTE request: Add handling and tests for LL_UNKNOWN_RSP #45244
Conversation
db5489a to
c71b661
Compare
|
Added commit that removes a test that was no longer valid one. More information in commit message. |
|
@thoh-ot @cvinayak @kruithofa @erbr-ot could you take a look on the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a inconsistency here with that fact that your are validating if an notification based on the LL_UNKNOWN_RSP is received but you are giving the validator an instance of a struct pdu_data_llctrl_reject_ext_ind as the reference, by chance the notification of type struct pdu_data_llctrl_unknown_rsp has a type field which actually matches the reject_opcode field of given remote_reject_ext_ind. I guess this was not the intended behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @thoh-ot! Good catch. It should be an address to unknown_rsp instance.
It is fixed now.
4b95ab2 to
f278b4b
Compare
f278b4b to
6f702d9
Compare
|
I have to rebase and fix merge conflicts.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont you want to check if feature exchange is complete? Legacy had a flag to indicate the features bitmap was valid or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; llcp.fex.features_used is initialized to the features of the local controller (LL_FEAT) and without checking llcp.fex.valid you cannot assume anything about the peer controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also look into ull_llcp_features.h and add any missing feature helper functions for CTE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is wrong. There are two ways to get information that peer does not support CTE response feature. By use of feature exchange procedure or reception of LL_UNKNOWN_RSP. I've did it wrong here and also in places where local feature is disabled. The if here should check whether feat. exchange is done and if peer supports CTE RSP, or if the local CTE REQ is still enabled.
If a device receives LL_UNKNOWN_RSP as a response for previous CTE REQ then it should disable local CTE REQ feature for the connection to do not allow for new requests.
6f702d9 to
ab9bb65
Compare
|
Fixed issue found by @cvinayak related with support for CTE RSP by peer device. |
There was missing handling of LL_UNKNOWN_RSP in CTE request control procedure.In case there is a pending CTE request and peer responses with LL_UNKNOWN_RSP then Host should be notified with HCI_LE_CTE_- Request_Failed event. The pending CTE request procedure should be completed. Signed-off-by: Piotr Pryga <[email protected]>
The CTE request procedure should be enabled only if a peer supports CTE response feature. That information can be obtained by feature exchange procedure. If there were no feature exchange then CTE request feature may be disabled if a peer responses with LL_UNKNOWN_RSP for a CTE request. The implementation of ll_df_set_conn_cte_req_enable was checking if CTE response feature is supported only when there was feature exchange. There was missing possibility to stop CTE request if a peer responded with LL_UNKNOWN_RSP for an earlier CTE request. The commit changes the implementation of ll_df_set_conn_cte_req_enable. The CTE response feature check is moved to ull_cp_cte_req function, because it belongs more to control procedure than to function that handles Host request to start the procedure. Second change is related with use of conn->llcp.fex.features_used. It stores information about features supported by peer. It does not depend on execution of the feature exchange control procedure. By the way, there were removed else statement in ll_df_set_conn_cte_- req_enable because it was not needed. Signed-off-by: Piotr Pryga <[email protected]>
Add test that verify correct behavior of the CTE request procedure in case of reception of LL_UNKNOWN_RSP PDU from peer device. Signed-off-by: Piotr Pryga <[email protected]>
There was a test case that was never executed. It was not added to a test suite in test_main. The commit adds the test case. Signed-off-by: Piotr Pryga <[email protected]>
There were couple of test cases where Host notification object was not released. The commit adds missing release calls. Also commnets related with check if RX queue is empty were changed to describe what is verified then. Signed-off-by: Piotr Pryga <[email protected]>
There was missing code responsible for handling of unexpected response for CTE request. The commit adds code that will terminate connection in case a peer device reposnes with unexpected control PDU that is not a remote procedure reques. Signed-off-by: Piotr Pryga <[email protected]>
There is a test that verifies a behavior of CTE request control procedure if receives LL_UNKNOWN_RSP. The expected behavior is to terminate connection if that is response handling is not implemented by a command. Other unexpected responses are treated as new remote control procedures. The CTE request has implemented handling of LL_UNKNOWN_RSP, hence this particular test is no longer valid. There is still open question how to handle other unexpected reposne PDUs while waiting for LL_CTE_RSP. That is not defined by BT 5.3. Core specification and not decided by community. The test is removed to because it fails and blocks CI. Signed-off-by: Piotr Pryga <[email protected]>
Due to change in Kconfig CONFIG_BT_CTLR_DF_MAX_ANT_SW_PATTERN_LEN the Kconfigs in tests were wrong and tests were not building. The commit sets the max antenna switch pattern length to actual maximum value. Signed-off-by: Piotr Pryga <[email protected]>
There was missing information about supported CTE response feature by peer device. That caused a test_set_conn_cte_req_enable to fail. The test expected that peer device supprots CTE response. The missing information is added in a common code responsible for connection object preparation. Signed-off-by: Piotr Pryga <[email protected]>
There was missing assignment to a global variable that stores connection handle. The value was always the same, hence it test were not failing. The value was passed over from setup state done for other tests. This change makes sure there is always correct handle stored in global variable. Signed-off-by: Piotr Pryga <[email protected]>
ab9bb65 to
b33b05d
Compare
|
I've found an issue with use of BTW. I've added comments to |
There was missing handling of LL_UNKNOWN_RSP in CTE request control
procedure. In case there is a pending CTE request and peer responses
with LL_UNKNOWN_RSP then Host should be notified with HCI_LE_CTE_-
Request_Failed event. The pending CTE request procedure should be
completed.
The CTE request procedure should be enabled only if a peer supports
CTE response feature. That information can be obtained by feature
exchange procedure. If there were no feature exchange then CTE
request feature may be disabled if a peer responses with LL_UNKNOWN_RSP
for a CTE request.
The implementation of ll_df_set_conn_cte_req_enable was checking if
CTE response feature is supported only when there was feature exchange.
There was missing possibility to stop CTE request if a peer responded
with LL_UNKNOWN_RSP for an earlier CTE request.
The commit changes the implementation of ll_df_set_conn_cte_req_enable.
The CTE response feature check is moved to ull_cp_cte_req function,
because it belongs more to control procedure than to function that
handles Host request to start the procedure.
Second change is related with use of conn->llcp.fex.features_used.
It stores information about features supported by peer. It does
not depend on execution of the feature exchange control procedure.
By the way, there were removed else statement in ll_df_set_conn_cte_-
req_enable because it was not needed.
Add test that verify correct behavior of the CTE request procedure
in case of reception of LL_UNKNOWN_RSP PDU from peer device.
Fixes not working CTE request control procedure unit tests.
Adds small refactoring related changes to CTE request control procedure unit tests.
There was missing code responsible for handling of unexpected response
for CTE request. The commit adds code that will terminate connection
in case a peer device responses with unexpected control PDU that is not
a remote procedure request.
Fixes: #45708.